Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

lalitb · 2024-10-11T19:35:51Z

Changes

changes similar to #2195 for TracerProvider

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

codecov · 2024-10-11T19:39:07Z

Codecov Report

Attention: Patch coverage is 95.83333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.2%. Comparing base (e6965bd) to head (21fa84e).

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/trace/provider.rs	95.8%	5 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #2197    +/-   ##
======================================
  Coverage   79.2%   79.2%            
======================================
  Files        122     122            
  Lines      20986   21089   +103     
======================================
+ Hits       16623   16723   +100     
- Misses      4363    4366     +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

opentelemetry-sdk/src/trace/provider.rs

cijothomas

Lets make it similar to #2195

…TracerProviderInner::Shutdown

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

lalitb · 2024-10-14T17:01:59Z

Lets make it similar to #2195

Done.

opentelemetry-sdk/src/trace/provider.rs

utpilla · 2024-10-15T23:40:42Z

opentelemetry-sdk/src/trace/provider.rs

+            drop(provider3);
+
+            // Verify shutdown was called exactly once
+            assert!(assert_handle.0.is_shutdown.load(Ordering::SeqCst));


How does this verify that shutdown was called only once? It looks like it's only verifying that shutdown was called (could have been called once or multiple times)

Good point. I think I should be using CountingShutdownProcessor which was added in #2195.

Have made this use CountingShutdownProcessor now.

opentelemetry-sdk/src/trace/provider.rs

utpilla · 2024-10-15T23:44:10Z

opentelemetry-sdk/src/trace/provider.rs

+            // Drop providers without explicit shutdown
+            drop(provider);
+            drop(provider2);
+


We should verify that shutdown was not called by asserting that assert_handle.0.is_shutdown is still false.

please have a look again, the tests are more focused now.

utpilla · 2024-10-15T23:45:52Z

opentelemetry/src/trace/mod.rs

@@ -200,6 +200,10 @@ pub enum TraceError {
    #[error("Exporting timed out after {} seconds", .0.as_secs())]
    ExportTimedOut(time::Duration),

+    /// already shutdown error
+    #[error("{0} already shutdown")]
+    AlreadyShutdown(String),


Do we expect to use this variant for anything other than TracerProvider?

Yes, thought to use them for the processors and exporters too. But I believe we can customize it later if required. For now, made it static for TracerProvider.

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

opentelemetry-sdk/src/trace/provider.rs

cijothomas · 2024-10-17T18:52:08Z

opentelemetry-sdk/src/trace/provider.rs

+///
+/// ## Cloning and Shutdown
+///
+/// The `TracerProvider` is designed to be lightweight and clonable. Cloning a `TracerProvider`


I don't think TracerProvider is lightweight. It is pretty heavy, and we expect user to create it only once. It is correct to mention cloning is cheap as it is just creating a new ref.

opentelemetry-sdk/src/trace/provider.rs

cijothomas · 2024-10-17T18:53:59Z

opentelemetry-sdk/src/trace/provider.rs

+///
+/// The `TracerProvider` manages the lifecycle of span processors, which are responsible for
+/// collecting, processing, and exporting spans. To ensure all spans are processed before shutdown,
+/// users can call the [`force_flush`](TracerProvider::force_flush) method at any time to trigger


lets remove force_flush mention here. I have seen many users doing force_flush in their code (and block their threads).. Not sure why, but lets make sure official docs don't recommend it.

Have reworded it so that it doesn't look as recommendation. I think it's better to at-least document since we provide it.

/// ## Span Processing and Force Flush /// /// The `TracerProvider` manages the lifecycle of span processors, which are responsible for /// collecting, processing, and exporting spans. The [`force_flush`](TracerProvider::force_flush) method /// invoked at any time will trigger an immediate flush of all pending spans (if any) to the exporters. /// This will block the user thread till all the spans are passed to exporters

cijothomas · 2024-10-17T18:55:13Z

opentelemetry-sdk/src/trace/provider.rs

-/// `TracerProvider` is lightweight container holding pointers to `SpanProcessor` and other components.
-/// Cloning and dropping them will not stop the span processing. To stop span processing, users
-/// must either call `shutdown` method explicitly, or drop every clone of `TracerProvider`.
+/// `TracerProvider` is a lightweight container holding pointers to `SpanProcessor` and other components.


not introduced in this PR, but advertising TracerProvider as lightweight is incorrect, and can lead to users repeatedly creating them, instead of doing it once.

cijothomas · 2024-10-17T18:56:07Z

opentelemetry-sdk/src/trace/provider.rs

+
+    #[derive(Debug)]
+    struct CountingShutdownProcessor {
+        shutdown_count: Arc<Mutex<i32>>,


nit: Atomics maybe easier here

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

initial commit

32938be

lalitb requested a review from a team as a code owner October 11, 2024 19:35

lalitb changed the title ~~void redundant shutdown in TracerProvider::drop when already shut down~~ Avoid redundant shutdown in TracerProvider::drop when already shut down Oct 11, 2024

Merge branch 'main' into tracer-provider-drop-shutdown-check

01c970b

cijothomas reviewed Oct 11, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

user reusable shutdown

c08c055

cijothomas requested changes Oct 13, 2024

View reviewed changes

lalitb and others added 3 commits October 14, 2024 09:09

Merge branch 'main' into tracer-provider-drop-shutdown-check

82d2598

restructure TracerProviderInner::Drop, TracerProvider::Shutdown, and …

f270dcd

…TracerProviderInner::Shutdown

Merge branch 'tracer-provider-drop-shutdown-check' of github.com:lali…

c8f2166

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

lalitb and others added 2 commits October 15, 2024 18:33

Update opentelemetry-sdk/src/trace/provider.rs

1a3dd67

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

Update opentelemetry-sdk/src/trace/provider.rs

36012dc

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

lalitb and others added 2 commits October 17, 2024 11:28

fix unit test

0d7d1ab

Merge branch 'main' into tracer-provider-drop-shutdown-check

01234c5

cijothomas reviewed Oct 17, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 17, 2024

View reviewed changes

update doc

2279f3a

lalitb added 2 commits October 17, 2024 14:55

Merge branch 'tracer-provider-drop-shutdown-check' of github.com:lali…

293120b

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

update to atomics

21fa84e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

lalitb commented Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

cijothomas left a comment

lalitb commented Oct 14, 2024

utpilla Oct 15, 2024

lalitb Oct 16, 2024

lalitb Oct 17, 2024

utpilla Oct 15, 2024

lalitb Oct 17, 2024

utpilla Oct 15, 2024

lalitb Oct 17, 2024

cijothomas Oct 17, 2024

cijothomas Oct 17, 2024

lalitb Oct 17, 2024

cijothomas Oct 17, 2024

cijothomas Oct 17, 2024

lalitb Oct 17, 2024

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Are you sure you want to change the base?

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Conversation

lalitb commented Oct 11, 2024

Changes

Merge requirement checklist

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

cijothomas left a comment

Choose a reason for hiding this comment

lalitb commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 •

edited

Loading